Skip to content

Fix SQLite writer atomic cache publish#940

Open
WarGloom wants to merge 1 commit into
DeusData:mainfrom
WarGloom:codex/fix-sqlite-writer-atomic-publish
Open

Fix SQLite writer atomic cache publish#940
WarGloom wants to merge 1 commit into
DeusData:mainfrom
WarGloom:codex/fix-sqlite-writer-atomic-publish

Conversation

@WarGloom

@WarGloom WarGloom commented Jul 7, 2026

Copy link
Copy Markdown

Summary

  • write direct SQLite dumps to a sibling temp file and atomically publish them only after finalization succeeds
  • discard temp DBs on writer errors instead of replacing the live cache with partial output
  • remove destination -wal and -shm sidecars when installing the freshly rebuilt DB

Root Cause

The custom SQLite page writer opened the final cache DB with fopen(path, "wb"). In shared MCP sessions, that can truncate a live DB while SQLite readers still have the old file mapped, and it can leave old-generation WAL/SHM sidecars beside the rebuilt DB. The observed crash reached SQLite btreeInitPage during cbm_store_delete_file_hashes after the dump/reopen step.

Validation

  • git diff --check
  • ASAN_OPTIONS=detect_leaks=0 build/c/test-runner sqlite_writer
  • ASAN_OPTIONS=detect_leaks=0 build/c/test-runner pipeline
    • passes 217 tests
    • note: existing unrelated UBSAN warning remains in src/pipeline/pass_similarity.c:152
  • make -f Makefile.cbm build/c/codebase-memory-mcp
  • CBM_CACHE_DIR=/tmp/cbm-wiki-mcp-pr-full-20260707-1 build/c/codebase-memory-mcp cli --index-worker index_repository '{"repo_path":"/home/nikita/work/Projects/ai/wiki-mcp"}' --response-out /tmp/cbm-wiki-mcp-pr-full-20260707-1.response
    • completed with status:"indexed", nodes:2256, edges:12934

Fixes #897.

@DeusData DeusData added bug Something isn't working stability/performance Server crashes, OOM, hangs, high CPU/memory priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Jul 7, 2026
@DeusData

DeusData commented Jul 7, 2026

Copy link
Copy Markdown
Owner

Thanks for the SQLite writer publish fix. Triage: draft high-priority cache/store integrity bug.

Review will be cautious because this touches direct DB publication: temp-file atomicity, cleanup on failure, destination WAL/SHM sidecar removal, and live-reader behavior all need explicit tests. Please also rename the PR title before it is ready for review so it describes the change directly.

@WarGloom WarGloom force-pushed the codex/fix-sqlite-writer-atomic-publish branch from 123f501 to 82df8c2 Compare July 8, 2026 09:24
@WarGloom WarGloom changed the title [codex] fix sqlite writer db publish Fix SQLite writer atomic cache publish Jul 8, 2026
@WarGloom

WarGloom commented Jul 8, 2026

Copy link
Copy Markdown
Author

Addressed.

  • Renamed the PR to describe the change directly: "Fix SQLite writer atomic cache publish".
  • Added explicit regression coverage for the remaining publish-integrity cases:
    • sw_publish_failure_removes_temp_output verifies a failed destination replacement discards the temporary DB output.
    • sw_publish_preserves_live_reader verifies a POSIX reader with an open transaction keeps seeing the old DB while a new connection sees the atomically published DB after replacement.
  • Kept the existing coverage for pre-finalize non-truncation and destination -wal/-shm sidecar cleanup.

Local validation after rebasing onto current main:

  • ASAN_OPTIONS=detect_leaks=0 build/c/test-runner sqlite_writer
  • bash scripts/check-no-test-skips.sh
  • git diff --check

@WarGloom WarGloom force-pushed the codex/fix-sqlite-writer-atomic-publish branch from 82df8c2 to 076b9a9 Compare July 8, 2026 09:39
@WarGloom WarGloom marked this pull request as ready for review July 8, 2026 12:11
@WarGloom WarGloom requested a review from DeusData as a code owner July 8, 2026 12:11
@DeusData DeusData added this to the 0.9.1-rc milestone Jul 8, 2026
@DeusData

DeusData commented Jul 9, 2026

Copy link
Copy Markdown
Owner

This fixes a real and nasty class — truncating the live DB under concurrent readers (#897's btreeInitPage crash) — and temp-write + atomic publish + discard-on-error is the right design. The tests and the WAL/SHM sidecar cleanup are welcome. Three changes requested:

  1. Windows non-ASCII paths: the new code opens the temp file with raw fopen and publishes via MoveFileExA (ANSI). On Windows, both break for non-ASCII cache paths (e.g. a user profile with umlauts — the cache DB name derives from the repo path). Repo policy is cbm_fopen (UTF-8 → _wfopen) for any user/cache path; please use it for the temp + sidecar writes here, and use MoveFileExW with a UTF-8→wide conversion for the publish (yes, artifact.c has the same ANSI call from Fix Windows artifact refresh in write_file_atomic #492 — that's a latent bug we'd rather not duplicate; feel free to extract a shared cbm_rename_replace() helper and we'll fix artifact.c in a follow-up).

  2. Durability of the publish: there's no fflush + fsync before the rename on the POSIX side (Windows at least passes MOVEFILE_WRITE_THROUGH). Without it, a power loss can atomically publish an incompletely-written file — the exact corruption the temp+rename dance is meant to prevent. fflush(fp); fsync(fileno(fp)); before fclose in the finalize path closes it.

  3. Sidecar removal ordering: please confirm (and comment) that the destination -wal/-shm removal happens only on the success path relative to the rename — removing the WAL of a DB an old reader still has open is fine on POSIX (unlinked-but-open) but fails or misbehaves on Windows if a handle is held; a note on the expected ordering/failure mode would help the next reader.

With those, this is exactly the writer we want.

@WarGloom WarGloom force-pushed the codex/fix-sqlite-writer-atomic-publish branch 2 times, most recently from 069a79c to 2a19411 Compare July 12, 2026 19:39
Keep the live database visible until a synced temporary generation can
replace it. Preserve destination sidecars on replacement failure and use
UTF-8-safe Windows file operations.

Signed-off-by: Nikita Bige <wargloom@gmail.com>
@WarGloom WarGloom force-pushed the codex/fix-sqlite-writer-atomic-publish branch from 2a19411 to 45b9a5c Compare July 13, 2026 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. stability/performance Server crashes, OOM, hangs, high CPU/memory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Installing a fresh DB file leaves the old -wal/-shm behind — SQLite replays the stale WAL over the new file

2 participants